Skip to content

New interface for ECCORestoring #185

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 44 commits into from
Nov 6, 2024
Merged

Conversation

simone-silvestri
Copy link
Collaborator

@simone-silvestri simone-silvestri commented Sep 20, 2024

should close #177

This PR also includes a maxiter field to the ECCONetCDFbackend that controls the maximum number of iterations for the inpainting algorithm

@@ -20,43 +21,41 @@ import Oceananigans.OutputReaders: new_backend, update_field_time_series!
struct ECCONetCDFBackend{N} <: AbstractInMemoryBackend{Int}
start :: Int
length :: Int
maxiter :: Int
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we make it more clear this refers to inpainting?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have changed it to inpainting_maxiter. I am not super happy about it though. Do you have a suggestion for a name for this variable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about making a new struct, Inpainting, which has a property maxiter (and which may be generalized to include more settings in the future)

grid = nothing)
function ECCORestoring(metadata::ECCOMetadata;
architecture = CPU(),
time_indices_in_memory = 2, # Not more than this if we want to use GPU!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be backend?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the moment, the only supported backend is the ECCOBackendNetCDF. If we want to be able to preprocess data we can change this to accept a backend and support different backends.

dates,
mask,
rate = 1 / 1000.0,
maxiter = 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should the max iter be set to the order of the advection scheme? or something like that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure. I don't think it has to do with the model detail, it has more to do with the discrepancy of the bathymetry we use with the ECCO bathymetry

@simone-silvestri
Copy link
Collaborator Author

Should be ready to go as soon as the tests pass

Copy link

codecov bot commented Oct 30, 2024

Codecov Report

Attention: Patch coverage is 0% with 56 lines in your changes missing coverage. Please review.

Project coverage is 0.00%. Comparing base (cd2677b) to head (72cfb20).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/DataWrangling/ECCO/ECCO_restoring.jl 0.00% 28 Missing ⚠️
src/DataWrangling/ECCO/ECCO_mask.jl 0.00% 14 Missing ⚠️
src/DataWrangling/inpaint_mask.jl 0.00% 10 Missing ⚠️
src/DataWrangling/ECCO/ECCO_metadata.jl 0.00% 3 Missing ⚠️
src/DataWrangling/ECCO/ECCO.jl 0.00% 1 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##            main    #185   +/-   ##
=====================================
  Coverage   0.00%   0.00%           
=====================================
  Files         32      32           
  Lines       1779    1806   +27     
=====================================
- Misses      1779    1806   +27     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@simone-silvestri
Copy link
Collaborator Author

should be mergeable

test/runtests.jl Outdated
Comment on lines 49 to 52
include("test_bathymetry.jl")
CUDA.set_runtime_version!(v"12.6"; local_toolkit = true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you may want to keep the test file though

@simone-silvestri simone-silvestri merged commit cbd7a86 into main Nov 6, 2024
7 checks passed
@simone-silvestri simone-silvestri deleted the ss/ecco-restoring-interface branch November 6, 2024 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ECCORestoring can be used directly as a forcing
2 participants